Repository navigation
[DT-4263][1/7] Record a send id on each email row - #3149
Merged
Merged
Conversation
kevinmarete
marked this pull request as ready for review
October 10, 2026 05:31
kevinmarete
requested review from
otchet-broad and
rushtong
and
a balanced review from Copilot
and removed request for
a team
October 10, 2026 05:31
otchet-broad
approved these changes
Oct 12, 2026
otchet-broad
left a comment
Contributor
There was a problem hiding this comment.
Nice! I've left a note in a test class about coverage when a filter isn't registered (tests still pass) that can be addressed as you see fit.
rushtong
approved these changes
Oct 13, 2026
rushtong
left a comment
Contributor
There was a problem hiding this comment.
Looks good. No medium or high findings:
- The
EmailSendIdThreadLocal clears only in the response filter. Dropwizard reduces this risk. - The new
send_idcolumn has no index. Later PRs in the stack look up rows by send_id, so they need one. - An async email sender that is not wrapped in
EmailSendId.runwrites a nullsend_id. - No DAO test inserts a null
sendIdand then reads backsend_id IS NULL.testInsert_stores_the_send_idtests only a non-null UUID. The service test for null uses a mock DAO, so it does not use the database. This is a small gap, because the null inserts in the other DAO tests pass.
Contributor
Author
Every email a request writes now carries that request's send_id, so the email log can group a send exactly instead of inferring it from timestamps. Emails written outside a request, and every row before this change, keep a null send_id and fall back to the existing rules. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
sendDailyMessages runs the expiration notices and both digests on an executor, outside the request filter, so those rows were written with no send id. Each job now runs under its own through EmailSendId.run. Also gives the filter an explicit priority, and drops the constructor that let an insert omit the send id. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kevinmarete
force-pushed
the
km-dt-4263-email-send-id
branch
from
October 13, 2026 18:04
6fb44a7 to
a996af7
Compare
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Addresses
https://broadworkbench.atlassian.net/browse/DT-4263
Security risk: low — adds a nullable
send_idcolumn toemail_entity, filled with a random per-request UUID; no change to who receives email or who can read it.Summary
email_entityhas one row per recipient and nothing linking the rows of one send, so the email log (#3142) has to infer sends from a 10-minute gap between timestamps. Code review kept finding cases that inference gets wrong: overlapping batches, chains of repeat emails, sends split by the date range. This records the grouping when the emails are written instead.ThreadLocaland cleared on the response.EmailService.sendMessagewrites it on every row it inserts.send_id, email type and entity reference, and no send loop has to change.send_idnull. The email log will keep the current timestamp rule for those rows.EmailNotifierResource.sendDailyMessages, which submits the expiration notices and both digests to an executor. Each of those jobs runs under its own send id throughEmailSendId.run. The other executors (dashboards, dataset cleanup, reindexing) send no email.testFetchByCreateDate_with_limit_and_offsetto UTC: it read the local date but built UTC midnights, so it failed between 8pm and midnight Eastern.#3142 will switch its grouping to
send_idwhere it's set once this merges.Have you read CONTRIBUTING.md lately? If not, do that first.
🤖 Generated with Claude Code